Skip to content

Improve partial unification handling #9523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 17, 2020
Merged

Conversation

smarter
Copy link
Member

@smarter smarter commented Aug 10, 2020

Currently failing because it breaks shapeless.

@smarter
Copy link
Member Author

smarter commented Aug 10, 2020

@milessabin This PR broke shapeless compilation as described in 9e7cc10, do you have the time to try to diagnose/minimize what's going on here?

@milessabin
Copy link
Contributor

It looks like the sort of change which would be likely to create new ambiguities and I can imagine that breaking the Pure derivation.

I'm afraid I'm really not able to commit the time to look into this properly right now :-(

@smarter
Copy link
Member Author

smarter commented Aug 10, 2020

Understood, thanks for taking a look. I'll look into this a bit more but since I'd like to get this in sooner rather than later (since it's a net improvement, it's potentially disruptive, and it's exercised by cats), I will probably have to comment out some of the code related to Pure in the branch of shapeless in our community build.

@smarter
Copy link
Member Author

smarter commented Aug 10, 2020

test performance please

@dottybot
Copy link
Member

performance test scheduled: 3 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9523/ to see the changes.

Benchmarks is based on merging with master (74ba21c)

@smarter smarter force-pushed the partial-hk1 branch 3 times, most recently from 0e4a673 to fa3c400 Compare August 11, 2020 18:08
@smarter
Copy link
Member Author

smarter commented Aug 11, 2020

Good news! I was able to find the root cause of the issues i was seeing with shapeless: fb7bc51, now everything is green! ✨ (the diverging implicit issue is still concerning, but it seems to be a manifestation of something already present on master: #9504)

@smarter smarter marked this pull request as ready for review August 11, 2020 19:05
@smarter smarter requested a review from odersky August 11, 2020 19:05
Not only can this prevent some constraints from being inferred, it can
lead to the wrong constraints being inferred due to partial unification,
see comment.

This change broke one test, in i6565.scala we had:

  type Lifted[A] = Err | A
  extension [O, U](o: Lifted[O]) def map(f: O => U): Lifted[U] = ???

  val error: Err = Err()

  lazy val ok: Lifted[String] = {
    point("a").map(_ => if true then "foo" else error)
      // now fails because error does not have type String
  }

Because `isMatchingApply` can now deal with aliased types, when typing
the lambda we get the constraint `U <: String` from
`Lifted[U] <: Lifted[String]`, so `Err` is no longer a subtype of the
expected result type of the lambda. This can be fixed in a number of
ways: I changed the test to use `flatMap` instead of `map`, but one
could also remove the expected type, or replace it by
`Lifted[Lifted[String]` or `Err | String`.

I think the new behavior is arguably better since using type aliases now
gives you more control on how type inference proceeds, even if it means
that some things that used to typecheck don't anymore.

This change is also necessary to get shapeless to compile after the
following commit which makes partial unification work in more
situation.
@smarter
Copy link
Member Author

smarter commented Aug 14, 2020

the diverging implicit issue is still concerning, but it seems to be a manifestation of something already present on master: #9504

#9504 is fixed, but the implicit issue is still here, however the issue goes away if the parameter is not marked by-name now (it didn't before), and I've been able to find a similar issue on master: #9568

So far, like Scala 2, given:

  Either[Int, String] <:< ?F[String]

we were able to infer:

  ?F >: [X] =>> Either[Int, X]

However, in the inverse situation:

  ?F[String] <:< Either[Int, String]

Scala 2, but not Dotty, was able to infer:

  ?F <: [X] =>> Either[Int, X]

This commit fixes this by generalizing the partial unification logic to
be run both in `compareAppliedType2` and `compareAppliedType1`, this
broke a few tests:
- In `anykind.scala`, `kinder1` and `kinder2` became ambiguous, this is
  fixed by moving `kinder2` in a lower priority base trait.
- One of the implicit search in tests/neg/i3452.scala now goes into an
  infinite loop, I've disabled it for now. The issue seems to be related
  to by-name implicits as the divergence checker works when the by-name
  is removed from one of the implicit parameter, I've added the
  non-by-name version in the same file. I've also been able to reproduce the
  same symptoms on master and filed scala#9568.
If `canInstantiate` fails, we fallback to `compareLower` which itself
can fallback to `fourthTry`, so there's no need to fallback to
`fourhTry` inside `canInstantiate` itself.
It turns out that neg/t2712-2.scala actually works in Dotty due to
better type inference.
@odersky
Copy link
Contributor

odersky commented Aug 17, 2020

#9568 is fixed by #9575

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. I went through it in detail. It all makes sense to me.

* (even though the type constructor variable isn't actually unified but only
* has one of its bounds constrained), for background see:
* - The infamous SI-2712: https://github.com/scala/bug/issues/2712
* - The PR against Scala 2.12 implementing -Ypartial-unification: https://github.com/scala/scala/pull/5102
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice explanation!

@odersky odersky merged commit d5efc05 into scala:master Aug 17, 2020
@odersky odersky deleted the partial-hk1 branch August 17, 2020 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants